fixed issue with MLJFlux.train #113
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
=======================================
Coverage 96.47% 96.47%
=======================================
Files 21 21
Lines 595 595
=======================================
Hits 574 574
Misses 21 21 ☔ View full report in Codecov by Sentry. |
|
@MojiFarmanbar wait |
| @@ -1,8 +1,8 @@ | |||
| # This file is machine-generated - editing it directly is not advised | |||
|
|
|||
There was a problem hiding this comment.
@Rockdeldiablo, changing the julia version is correct?
There was a problem hiding this comment.
@MojiFarmanbar yes it shouldn't be a problem.
| else | ||
| la = chain | ||
| end | ||
|
|
||
| # Initialize history: |
There was a problem hiding this comment.
@Rockdeldiablo , I would suggest to keep this if-else on top as it was because it checks if chain is abstractlaplace or not. moving this piece down before LaplaceRedux.fit doesn't add any logics. if you agree i would suggest the followings:
- keeping that if-else as it was to check if chain is laplace object
- line 242 and 243, changing
chaintolain for-loop, it would solve the problem
what do you think?
There was a problem hiding this comment.
@MojiFarmanbar honestly i do not think it would work because
MLJFlux.train_epoch(
model, chain, regularized_optimiser, optimiser_state, X, y
) requires an actual chain not a laplace object
the laplace object is required only by LaplaceRedux.fit which is at the end.
have you tested it?
this pull request solve an issue that was affecting the MLJ interface that passed unnoticed in the previous pull request. In particular, the chain that was passed to mlj.predict was copied before the training phase instead of after. (part of gsoc 24)